Skip to content

Implement array_all type specifying for arrow functions#5134

Open
uekann wants to merge 17 commits intophpstan:2.1.xfrom
uekann:feature/type-specify-array-all
Open

Implement array_all type specifying for arrow functions#5134
uekann wants to merge 17 commits intophpstan:2.1.xfrom
uekann:feature/type-specify-array-all

Conversation

@uekann
Copy link
Copy Markdown

@uekann uekann commented Mar 5, 2026

This PR partially resolves phpstan/phpstan#12939 by specifying array element types when array_all returns true.

To keep this initial PR focused, it currently supports only ArrowFunction (e.g., array_all($arr, fn($v, $k) => is_int($v) && is_string($k))). It correctly recognizes union and intersection types when the arrow function uses || (or) and && (and).

The following features are left as future scope:

Standard Closures: (e.g., function($v) { ... })

String and First-class callables: (e.g., 'is_int' or is_int(...)). Note that these currently throw an ArgumentCountError in PHP anyway, as array_all() passes both value and key, but functions like is_int expect exactly one argument (see: https://3v4l.org/2DQ9a#veol).

Custom assertions: Support for @phpstan-assert-if-true and similar annotations is not included in this PR, as mapping variables to types in those contexts is currently difficult and requires further work.

@uekann
Copy link
Copy Markdown
Author

uekann commented Mar 5, 2026

I'm out this weekend and will get to any feedback on Monday.

@uekann uekann force-pushed the feature/type-specify-array-all branch from 2d9dbbb to e44e476 Compare March 9, 2026 04:57
@uekann uekann marked this pull request as draft March 9, 2026 04:59
@uekann uekann marked this pull request as ready for review March 9, 2026 05:20
@uekann uekann requested a review from staabm March 9, 2026 05:20
@phpstan-bot
Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@uekann uekann marked this pull request as draft March 9, 2026 08:38
@uekann uekann force-pushed the feature/type-specify-array-all branch from 845b403 to 407c372 Compare March 11, 2026 01:23
@uekann
Copy link
Copy Markdown
Author

uekann commented Mar 11, 2026

@staabm
Thank you for the review.
I made the following changes:

  • Added the test cases you suggested
  • Added support for Closure

I also considered implementing support so that passing other functions annotated with @phpstan-assert-if-true to array_all() would enable proper type narrowing. However, when such a function is passed to array_all() as a first-class callable, for example as f(...), it is treated as a Closure, and in PHPStan a Closure currently does not retain information equivalent to assert-if-true. For that reason, I have not implemented this at this time. (See: phpstan/phpstan#14249)

@uekann uekann marked this pull request as ready for review March 11, 2026 02:25
@phpstan-bot
Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@uekann uekann requested a review from staabm March 12, 2026 14:27
public function specifyTypes(FunctionReflection $functionReflection, FuncCall $node, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes
{
$args = $node->getArgs();
if (!$context->truthy() || count($args) < 2) {
Copy link
Copy Markdown
Contributor

@staabm staabm Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never know whether its $context->truthy() or $context->true().

please add a test with a === true comparison
(the escaped mutant at least tells us, its not covered by tests)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for the === true comparison, but for a function returning bool it does not distinguish between $context->true() and $context->truthy(): both lead to the same narrowing on the true branch. So the test covers the syntax, but it does not really prove which context check is the correct one here.

@@ -0,0 +1,179 @@
<?php // lint >= 8.4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this test really depend on php 8.4? I think this can be lowered without problems?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that array_all is a function added in PHP 8.4. Therefore, the tests are set for 8.4 and later.
I ran it on my machine and it passed on 8.3 (I'm not sure why this is), so should I remove the mention of 8.4 and later?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. it works on 8.3 because we have symfony/polyfills.

leave it as is than, thanks.

Comment on lines +49 to +54
$callable instanceof Expr\Closure &&
count($callable->stmts) === 1 &&
$callable->stmts[0] instanceof Stmt\Return_ &&
isset($callable->stmts[0]->expr)
) {
$callableExpr = $callable->stmts[0]->expr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right approach, but don't have a better idea atm either

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially considered performing type specification for arbitrary closures, but I concluded that this would require tracking the result of executing statements, which does not seem realistic. I also do not think that would align well with PHPStan’s design.

For that reason, I implemented it so that type specification is applied only to closures that contain nothing but a return statement.

Another option would be to apply type specification only to closures produced by converting a function to a first-class callable. I considered this because assertions written in PHPDoc can exist on the original function. However, at the time of implementation, that assertion information was lost when the function was converted to a closure, so this did not seem realistic either.

(The issue I opened about that, phpstan/phpstan#14249, has since been resolved, so this may become a viable option in the future. However, at least for now, I also cannot think of a better alternative, so I think this approach is acceptable as a temporary solution.)

@zonuexe
Copy link
Copy Markdown
Contributor

zonuexe commented Apr 2, 2026

@ondrejmirtes
Is there a possibility that this PR will be merged? While there are still some less-than-ideal aspects to the closure analysis, it seems to cover the realistic cases.

@staabm staabm force-pushed the feature/type-specify-array-all branch from c2e15f0 to 85be1cc Compare April 2, 2026 08:47
@staabm staabm force-pushed the feature/type-specify-array-all branch from 85be1cc to 14e0f3d Compare April 8, 2026 17:58
@staabm staabm requested a review from VincentLanglet April 8, 2026 17:58
Comment on lines +48 to +53
} elseif (
$callable instanceof Expr\Closure &&
count($callable->stmts) === 1 &&
$callable->stmts[0] instanceof Stmt\Return_ &&
isset($callable->stmts[0]->expr)
) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @staabm that this might not be satisfying.

The first thing I thought then was to check how it was done for array_filter (in ArrayFilterFunctionReturnTypeHelper) and they seems to do something similar

if ($callbackArg instanceof Closure && count($callbackArg->stmts) === 1 && count($callbackArg->params) > 0) {
			$statement = $callbackArg->stmts[0];
			if ($statement instanceof Return_ && $statement->expr !== null) {

But it handles also

 elseif (
			($callbackArg instanceof FuncCall || $callbackArg instanceof MethodCall || $callbackArg instanceof StaticCall)
			&& $callbackArg->isFirstClassCallable()
		)

and

else {
			$constantStrings = $scope->getType($callbackArg)->getConstantStrings();
			if (count($constantStrings) > 0) {

So I think we could have more case to handle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants